Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement new registration flow with email verification #5215

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Implement new registration flow with email verification #5215

wants to merge 4 commits into from
+255 −8

Conversation

dani-garcia
Copy link
Owner

New versions of the clients support a new registration flow that also allows enforcing email verification. This is a working implementation of that new API.

The clients implementations are gated behind two feature flags that I have enabled, I've used a web vault compiled from the main branch, but older releases might also work.

Still a draft as I still need to test Invites/emergency access

@dani-garcia dani-garcia changed the title Implement registration with required verified email Implement new registration flow with email verification Nov 21, 2024
@BlackDex BlackDex mentioned this pull request Feb 5, 2025
Open
stefan0xC
stefan0xC reviewed Feb 5, 2025
View reviewed changes
src/api/identity.rs Outdated Show resolved Hide resolved
@stefan0xC
Copy link
Contributor

Still a draft as I still need to test Invites/emergency access

Emergency access will fail

[2025-02-05 09:25:23.688][request][INFO] POST /identity/accounts/register/finish
[2025-02-05 09:25:23.688][vaultwarden::api::core::accounts][ERROR] Email verification token is required
[2025-02-05 09:25:23.688][response][INFO] (register_finish) POST /identity/accounts/register/finish => 400 Bad Request

There is an acceptEmergencyAccessInviteToken in the request though.

Same with invitations / orgInviteToken:

[2025-02-05 09:28:00.722][request][INFO] POST /identity/accounts/register/finish
[2025-02-05 09:28:00.722][vaultwarden::api::core::accounts][ERROR] Email verification token is required
[2025-02-05 09:28:00.722][response][INFO] (register_finish) POST /identity/accounts/register/finish => 400 Bad Request

@dani-garcia dani-garcia force-pushed the register_verify_email branch from 23b0863 to efc82da Compare February 5, 2025 22:49
@dani-garcia
Copy link
Owner Author

Thanks for the testing @stefan0xC , I've fixed emergency access now and I'll look into the org invites tomorrow.

@dani-garcia dani-garcia marked this pull request as ready for review February 5, 2025 22:51
@stefan0xC
Copy link
Contributor

stefan0xC commented Feb 6, 2025 •
edited
Loading

@dani-garcia I'm not sure if accepting the emergency access invitation in _register

vaultwarden/src/api/core/accounts.rs

Line 329 in efc82da

emergency_access.accept_invite(&user.uuid, &user.email, &mut conn).await?;
is necessary because it seems like the client will sent a separate request after registration is finished to accept the emergency access anyway (which will then fail because there is no pending emergency access request to be accepted anymore):

[2025-02-06 07:46:59.770][request][INFO] POST /identity/accounts/register/finish
[2025-02-06 07:47:02.319][response][INFO] (register_finish) POST /identity/accounts/register/finish => 200 OK
[2025-02-06 07:47:02.481][request][INFO] POST /identity/accounts/prelogin
[2025-02-06 07:47:02.482][response][INFO] (prelogin) POST /identity/accounts/prelogin => 200 OK
[2025-02-06 07:47:02.652][request][INFO] POST /identity/connect/token
[2025-02-06 07:47:04.890][vaultwarden::api::identity][INFO] User [...] logged in successfully. IP: 192.168.1.100
[2025-02-06 07:47:04.891][response][INFO] (login) POST /identity/connect/token => 200 OK
[2025-02-06 07:47:05.076][request][INFO] GET /api/config
[2025-02-06 07:47:05.076][response][INFO] (config) GET /api/config => 200 OK
[2025-02-06 07:47:05.292][request][INFO] POST /identity/connect/token
[2025-02-06 07:47:05.294][response][INFO] (login) POST /identity/connect/token => 200 OK
[2025-02-06 07:47:05.319][request][INFO] GET /notifications/hub?access_token=eyJ0eXAiOiJKV1QiL
[2025-02-06 07:47:05.320][vaultwarden::api::notifications][INFO] Accepting Rocket WS connection from 192.168.1.100
[2025-02-06 07:47:05.320][response][INFO] (websockets_hub) GET /notifications/hub?<data..> => 200 OK
[2025-02-06 07:47:05.446][request][INFO] GET /api/sync?excludeDomains=true
[2025-02-06 07:47:05.448][response][INFO] (sync) GET /api/sync?<data..> => 200 OK
[2025-02-06 07:47:05.955][request][INFO] POST /api/emergency-access/0ac804b7-232d-44c8-b701-ae2615a07eba/accept
[2025-02-06 07:47:05.957][vaultwarden::api::core::emergency_access][ERROR] Emergency access not valid.
[2025-02-06 07:47:05.963][response][INFO] (accept_invite) POST /api/emergency-access/<emer_id>/accept => 400 Bad Request
[2025-02-06 07:47:06.558][request][INFO] GET /api/accounts/revision-date
[2025-02-06 07:47:06.559][response][INFO] (revision_date) GET /api/accounts/revision-date => 200 OK
[2025-02-06 07:47:06.570][request][INFO] GET /api/accounts/profile
[2025-02-06 07:47:06.571][response][INFO] (profile) GET /api/accounts/profile => 200 OK

@dani-garcia
Copy link
Owner Author

Oops didn't notice the client was doing the accept right afterwards, same thing with the org invites so the only thing I do is validate the claims in the token match with the request now.

stefan0xC
stefan0xC suggested changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@stefan0xC stefan0xC left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dani-garcia I could not yet accept an organization invite. With the new registration flow a token will not be send anymore so we should change it to org_invite_token accordingly.

src/api/core/accounts.rs
if Invitation::take(&email, &mut conn).await
|| CONFIG.is_signup_allowed(&email)
|| pending_emergency_access.is_some()
|| pending_org_invite.is_some()
Copy link
Contributor

@stefan0xC stefan0xC Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When inviting an User the entry in the table will already have been created, so this should not be necessary, I think.

With the new signup form will a data.token ever get passed? If not

vaultwarden/src/api/core/accounts.rs

Lines 235 to 236 in 60a9474

if let Some(token) = data.token {
let claims = decode_invite(&token)?;
should probably be changed to data.org_invite_token instead. (I mean, we'd probably want to keep it for backwards compatibility but accepting an invitation currently fails.)

src/api/core/accounts.rs
email_verification_token: Option<String>,
accept_emergency_access_id: Option<EmergencyAccessId>,
accept_emergency_access_invite_token: Option<String>,
org_invite_token: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get rid of token and instead make it an alias for backwords compatibility?

Suggested change
org_invite_token: Option<String>,
#[serde(alias = "token")]
org_invite_token: Option<String>,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I've tested it that seems to work both with web-v2025.1.1 (with and without the feature flags set) and web-v2025.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefan0xC stefan0xC stefan0xC requested changes

@BlackDex BlackDex Awaiting requested review from BlackDex

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dani-garcia @stefan0xC